feat(simulators): make clear what is the default value of the sort argument in simulation runs list#2695
feat(simulators): make clear what is the default value of the sort argument in simulation runs list#2695MortGron wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request changes the default value of the 'sort' parameter in the simulator runs 'list' method from 'None' to 'SimulationRunsSort()'. The reviewer advises against using a complex object as a default argument in the function signature, as it is a Python anti-pattern and introduces a breaking change for callers explicitly passing 'None'. Instead, the reviewer recommends keeping 'sort: SimulationRunsSort | None = None' in the signature, handling the default value inline within the function body, and updating the docstrings to reflect this.
| created_time: TimestampRange | None = None, | ||
| simulation_time: TimestampRange | None = None, | ||
| sort: SimulationRunsSort | None = None, | ||
| sort: SimulationRunsSort = SimulationRunsSort(), |
There was a problem hiding this comment.
Avoid using mutable or complex objects like SimulationRunsSort() as default arguments in function signatures. This is a well-known Python anti-pattern because default arguments are evaluated once at function definition time, which can lead to shared state bugs. Additionally, removing None from the allowed types of sort is a breaking change for any caller explicitly passing None (e.g., when forwarding arguments).
Please keep sort: SimulationRunsSort | None = None in the signature and handle the default value inside the function body.
| sort: SimulationRunsSort = SimulationRunsSort(), | |
| sort: SimulationRunsSort | None = None, |
References
- Consistency: Follow established patterns across the codebase. Other list methods and call use
sort: SomethingSort | None = None. (link)
There was a problem hiding this comment.
If we wanted to enforce this rule, we could enable https://docs.astral.sh/ruff/rules/mutable-argument-default/#mutable-argument-default-b006. However, is not currently enabled. Also, the function does not mutate this argument, so there is no risk.
The change is breaking, but simulators is in alpha.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2695 +/- ##
==========================================
- Coverage 93.70% 93.66% -0.04%
==========================================
Files 503 503
Lines 50699 50699
==========================================
- Hits 47505 47488 -17
- Misses 3194 3211 +17
🚀 New features to boost your workflow:
|
1 participant
Description
Make clear what is the default sort order for simulations runs list.
Checklist:
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.